feat!: fail if we know the project is core24 #4557

tigarmo · 2024-02-01T21:06:04Z

The purpose of this PR is two-fold:

Add breaking code that checks whether the project is known to be core24, to fail early. Otherwise, if the project has a different base (or if there's no project) then we fallback to the current codepath as usual.
Add a set of core24 spread tests, each with an expected failure.

A lot of the changes are copied over from existing tests, so some commits can mostly be skimmed. I tried to split them up in a way that makes functional sense (to me at least):

First commit has the actual feature of checking the base and adding a new RuntimeError;
Second commit adds core24 spread tests, but as a straight-up copy of the existing tests in tests/spread/core22/. These tests are unchanged and still have core22 as base, so they don't need a big review;
Third commit has the mostly-automated change of base of the core24 tests. I think it's worth a look over because I had to do a few manual changes;
The fourth commit adds all expected failures in these new tests because none of them passes cleanly. For example, all tests that call snapcraft pack are expected to fail with "pack" command is not implemented for core24. The tests now check for this message and exit.
Finally, the fifth commit restricts the CI spread runs to only tests/spread/core24. I confirmed that the other tests are passing, and restricting this feature-branch to only core24 tests speeds up the development loop considerably.

codecov-commenter · 2024-02-01T21:22:54Z

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (9855085) 88.33% compared to head (0abe95e) 88.20%.

❗ Current head 0abe95e differs from pull request most recent head d8567cd. Consider uploading reports for the commit d8567cd to get more accurate results

Files	Patch %	Lines
snapcraft/application.py	0.00%	28 Missing ⚠️
snapcraft/commands/unimplemented.py	0.00%	6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                      Coverage Diff                      @@
##           feature/craft-application    #4557      +/-   ##
=============================================================
- Coverage                      88.33%   88.20%   -0.13%     
=============================================================
  Files                            327      327              
  Lines                          21961    21992      +31     
=============================================================
  Hits                           19399    19399              
- Misses                          2562     2593      +31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

snapcraft/application.py

This commit updates the scaffolding to have a different behavior in cases where we *know* that the existing project (from the yaml file) is core24-based. In these cases, we now raise a RuntimeError outlining the name of the command instead of raising a ClassicFallback error to let the non-craft-application workflow take over. With this, Snapcraft runs on core24 projects will fail until the commands are ported over, while projects for older bases (or commands that don't use projects, like "init") will still fallback to the existing behavior.

These are all the tests from tests/spread/core22, copied over to tests/spread/core24. Completely unmodified, they therefore still have the core22 base.

This is a semi-manual update of "base" to "core24", "build-base" to "devel" and "grade" to "devel" (where applicable).

This commit does the equivalent of pytest's "xfail(strict=True)": all core24 tests are currently fail because no Snapcraft command has been implemented yet, so the test checks that the tool fails with the expected message, and then exits successfully. The idea is that once a command is implemented the corresponding spread tests will start to fail - we can then scrutinize each test to validate the new behavior.

tigarmo · 2024-02-07T11:44:07Z

I had to force-push this to re-seat it on the new feature/craft-application branch. Sorry!

mr-cal

Excellent work! Looks like we will have to adjust the required CI jobs for this branch

tests/spread/core24/architectures/task.yaml

The intention here is to speed-up spread runs and make the development loop faster. I've run the non-core24-tests in this feature branch and can confirm that they pass (or have unrelated failures).

tigarmo · 2024-02-08T18:58:35Z

Excellent work! Looks like we will have to adjust the required CI jobs for this branch

thanks! I updated the workflow to keep the name of the required step

tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch 6 times, most recently from f793720 to 4fb3c6b Compare February 6, 2024 11:35

tigarmo changed the base branch from main to feature/craft-application February 6, 2024 11:35

tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch from 4fb3c6b to 1372d07 Compare February 6, 2024 12:13

tigarmo marked this pull request as ready for review February 6, 2024 16:16

syu-w reviewed Feb 6, 2024

View reviewed changes

snapcraft/application.py Show resolved Hide resolved

Base automatically changed from feature/craft-application to main February 7, 2024 11:10

tigarmo added 4 commits February 7, 2024 08:30

chore(spread): add many core24 tests

a558fd9

These are all the tests from tests/spread/core22, copied over to tests/spread/core24. Completely unmodified, they therefore still have the core22 base.

chore(spread): fix bases in core24 tests

14e20be

This is a semi-manual update of "base" to "core24", "build-base" to "devel" and "grade" to "devel" (where applicable).

tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch from e76e2fd to 0abe95e Compare February 7, 2024 11:31

tigarmo changed the base branch from main to feature/craft-application February 7, 2024 11:31

tigarmo requested review from lengau and syu-w February 7, 2024 20:49

syu-w approved these changes Feb 7, 2024

View reviewed changes

tigarmo requested a review from mr-cal February 8, 2024 15:26

mr-cal approved these changes Feb 8, 2024

View reviewed changes

tests/spread/core24/architectures/task.yaml Show resolved Hide resolved

ci(spread): only run core24 spread tests

d8567cd

The intention here is to speed-up spread runs and make the development loop faster. I've run the non-core24-tests in this feature branch and can confirm that they pass (or have unrelated failures).

tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch from 0abe95e to d8567cd Compare February 8, 2024 18:58

tigarmo merged commit b22fe93 into feature/craft-application Feb 8, 2024
9 of 10 checks passed

tigarmo deleted the CRAFT-2441-craft-app-core24-only branch February 8, 2024 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: fail if we know the project is core24 #4557

feat!: fail if we know the project is core24 #4557

tigarmo commented Feb 1, 2024 •

edited

Loading

codecov-commenter commented Feb 1, 2024 •

edited

Loading

tigarmo commented Feb 7, 2024

mr-cal left a comment

tigarmo commented Feb 8, 2024

feat!: fail if we know the project is core24 #4557

feat!: fail if we know the project is core24 #4557

Conversation

tigarmo commented Feb 1, 2024 • edited Loading

codecov-commenter commented Feb 1, 2024 • edited Loading

Codecov Report

tigarmo commented Feb 7, 2024

mr-cal left a comment

Choose a reason for hiding this comment

tigarmo commented Feb 8, 2024

tigarmo commented Feb 1, 2024 •

edited

Loading

codecov-commenter commented Feb 1, 2024 •

edited

Loading